Clean up Jekyll file types and site #1300
Conversation
|
I appreciate your efforts to help clean up our codebase and I do like what you've done with several commits in this pull request. For example, 303bd56 makes that variable name much clearer and 2408c75 combines two operations into one, which is almost never a bad thing. 😃 I have a lot of questions about what you're trying to do with some of the changes you made though. Why remove the LSI messages in 3113b61? They provide meaningful output for an operation that can take a long time. Why raise exceptions in 6d098d7? Why remove unnecessary Why combine Post, Page, and StaticFile? (0648d5f and e188911) I know my various attempts at refactoring the codebase haven't gone well. A lot of this seems like you're just trying to remove the number of lines of code across the codebase vs. making targeted structural changes that improve design w/o affecting functionality. I looked through all the commits and they're all one line with no additional context to explain the change. Could you help me understand the thought process behind these changes? |
|
@mattr- sorry don't mean to drop a big pull without explanation :(. I'll rebase them to have more explanatory commit comments. I have the bad habit of leading with code and then following with an explanation.
I wouldn't say the goal is line numbers but complexity. Site.rb is a monster that seems to be concerned with a lot of stuff and lacking a single responsibility. I think it's one responsibility should be managing the site files and a lot of the changes flow from that.
I wasn't sure why it was there. I guessed it was because it might take a long time. I was bold and took it out and figured if it was useful I would reopen the commit and edit the messages to say it would take a long time. It's also sort of surprising to have them write to the console. Especially if jekyll is include within something else.
Tell Don't Ask - decisions about the state of the object should be made in the objects themselves. If Post is used outside of Site it should still tell you it can't be made if the name is wrong. I'll go through and expand the other ones. Sorry again :( don't mean to come across as too much of a cowboy |
|
Sure... I will take a look @jpiasetz 😃 Thanks for the effort on reducing complexity (= |
| @@ -26,18 +26,18 @@ def to_s | |||
| # name - The String filename of the file. | |||
| # | |||
| # Returns nothing. | |||
| def read_yaml(base, name) | |||
| def read_yaml(path) | |||
There was a problem hiding this comment.
Good change, I always consider reducing param quantity a good thing... Since it continues making sense... As in this case...
Just only remember to update the TomDoc above (=
|
@jpiasetz, still you there? |
|
@parkr this just too out of daete? |
|
Nope I deleted |
|
No worries haha. I probably should get back on fixing these up. |
|
Thanks @jpiasetz :) |
|
Also rebasing on master would be ✨ |
…d accident shadowing.
…rong. Do it as soon as you can.
… is using relative_permalinks is page's concern not site's.
…dy beening passed as part of site)
…. At the end of the chain read_yaml load off a path. Why split it up in site only to join it back together in the end? Also why is it site's responsibility to break up a path? Let the object break up the path how they want. Refactor the read methods into one to avoid recursion and modify read_yaml to take a path. The multi reads seemed to be a side effect of having to break the path up so join them back together.
…ponsibilities should be dealing with the files not dealing with posts, pages, layouts, static_files,..., rss_files, images, etc. If more types get added would variables just keep getting made for them? Make the variables reflect responsibilities.
…ite to the console status updates by default? Is it because it's expected to take a long time? There's no way to turn it off and it could expected behaviour if jekyll is included somewhere else.
… the conditional to the class - Tell Don't Ask, decision about the state of the object should be made in the objects themselves. It cut's down on coupling and makes it easier to change. If you use the Post class by outsite of site it should still tell you it can't be made with an invalid name.
|
Bah this is in a big mess. On the plus side looks like lots of the stuff I was suggesting already has been done 👍. I'll work through cleaning up my crappy rebase and then figure out what still applies. |
|
With collections, I'd eventually like to clean up Posts and Drafts so they are part of collections, just automatically read-in rather than requiring the user to specify them in their config. If we can work together to make |
😍 💓 👍 |
|
@jpiasetz Documents will eventually be abstracted to posts and pages but not until 3.0.0, which will be a huuuuuge internal rewrite. |
|
@jpiasetz Would love your expertise on merging the pages/posts concepts into the collection concept. Ideally we'd 🐶food everything so pages and posts are just collections. Feeling a bit like |
|
Let's revisit this in a different PR. :) |
I tried to clean up the Pages, Posts, Layouts and StaticFiles and decouple them from site. @parkr I ran ruby-prof against it https://gist.github.com/5979489
@kelvinst you might want to take a look.